Skip to content

feat(utils): the utils now contain a service that wraps around the BroadcastChannel API #253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Oct 17, 2024
Merged

feat(utils): the utils now contain a service that wraps around the BroadcastChannel API #253

merged 4 commits into from
Oct 17, 2024

Conversation

@DenisValcke
Copy link
Contributor

This PR adds the BroadcastChannelService to the utils. This service wraps around the BroadcastChannel API:
https://developer.mozilla.org/en-US/docs/Web/API/Broadcast_Channel_API

@DenisValcke DenisValcke added enhancement New feature or request ngx-utils @studiohyperdrive/ngx-utils labels Oct 17, 2024
@DenisValcke DenisValcke added this to the ngx-utils:18.4.0 milestone Oct 17, 2024
@DenisValcke DenisValcke self-assigned this Oct 17, 2024
IbenTesara
IbenTesara suggested changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@IbenTesara IbenTesara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, mostly to keep it in line with the other packages.

Maybe also update the tags in the package.json?

libs/utils/src/lib/services/broadcast-channel/broadcast-channel.service.ts Outdated
return;
}

if (!this.broadcastChannel[channelName]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well, I'd expect an error not a warn. I'd also refer to the initChannel method here.

ian-emsens-shd
ian-emsens-shd requested changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@ian-emsens-shd ian-emsens-shd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Service does not implement the whole spec and lacks automatic type inference. Comment about constructor argument signature also applies to function parameters to a lesser extent.

WHeirstrate
WHeirstrate reviewed Oct 17, 2024
View reviewed changes
@DenisValcke
Copy link
Contributor Author

DenisValcke commented Oct 17, 2024 •
edited
Loading

To recap the feedback into a todo list:

  • Add tags to the package.json in line with the other packages
  • Add the service to the global README of the ngx-utils
  • Fix a typo in the service's README: it takes SSR into account
  • Simplify the checks to see if the channelName is provided and exists within the Record
  • Fix the selectChannel method mismatch with the documentation
  • Convert console.warn messages to console.error and format the messages to be in line with others
  • Rework the constructor to be more future proof for multiple arguments
  • Add a method for the messageerror event to fully support the spec
  • OPTIONAL: use the window service to check if the platform is browser

Additional:

  • Rename BroadcastChannelService to NgxBroadcastChannelService to match other services

@DenisValcke
feat(utils): several improvements have been made to the NgxBroadcastC…
60a7b44 
…hannelService including simplified safety and fully supporting the spec
@DenisValcke
Copy link
Contributor Author

All feedback has been processed (see checklist).

IbenTesara
IbenTesara approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@IbenTesara IbenTesara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still one BroadcastChannelService instead of NgxBroadcastChannelService in the general docs but looks good to me!

@DenisValcke
Copy link
Contributor Author

DenisValcke commented Oct 17, 2024 •
edited
Loading

  • Update the NgxBroadcastChannelService README to reflect recent changes
  • Update the NgxUtils README to reflect recent changes

@DenisValcke DenisValcke merged commit c8814cb into master Oct 17, 2024
@DenisValcke DenisValcke deleted the feature/utils--broadcast-channel-service branch October 17, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WHeirstrate WHeirstrate WHeirstrate left review comments

@ian-emsens-shd ian-emsens-shd ian-emsens-shd approved these changes

@ekincia ekincia ekincia approved these changes

+1 more reviewer

@IbenTesara IbenTesara IbenTesara approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@DenisValcke DenisValcke

Labels

enhancement New feature or request ngx-utils @studiohyperdrive/ngx-utils

Projects

None yet

Milestone

ngx-utils:18.4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@DenisValcke @IbenTesara @WHeirstrate @ian-emsens-shd @ekincia